Skip to content

[SPARK-8658] [SQL] [FOLLOW-UP] AttributeReference's equals method compares all the members#9761

Closed
gatorsmile wants to merge 2 commits intoapache:masterfrom
gatorsmile:hashCodeNamedExpression
Closed

[SPARK-8658] [SQL] [FOLLOW-UP] AttributeReference's equals method compares all the members#9761
gatorsmile wants to merge 2 commits intoapache:masterfrom
gatorsmile:hashCodeNamedExpression

Conversation

@gatorsmile
Copy link
Member

@gatorsmile gatorsmile commented Nov 17, 2015

Based on the comment of @cloud-fan in #9216, update the AttributeReference's hashCode function by including the hashCode of the other attributes including name, nullable and qualifiers.

Here, I am not 100% sure if we should include name in the hashCode calculation, since the original hashCode calculation does not include it.

@marmbrus @cloud-fan Please review if the changes are good.

@SparkQA
Copy link

SparkQA commented Nov 17, 2015

Test build #46069 has finished for PR 9761 at commit eb63f09.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@gatorsmile
Copy link
Member Author

gatorsmile commented Nov 17, 2015

@nongli I saw you have a related discussion with @chenghao-intel . The failed test case was introduced in your PR #9480. I am not sure the root reason why we intentionally exclude name from hashCode, but the original equals includes name. It breaks a general principle of hashCode function design:

An object’s hashCode method must take the same fields into account as its equals method.

Based on my understanding, in a case-insensitive HiveContext, we still should detect their differences when the case of name is different but the exprId values are the same

@nongli
Copy link
Contributor

nongli commented Nov 17, 2015

This change looks good to me. The failing test is not valid anymore and should be changed to verify that the hash codes are not equal.

@gatorsmile
Copy link
Member Author

gatorsmile commented Nov 17, 2015

Ok. I will also add three more lines for covering the new hashCode and equals functions.

@SparkQA
Copy link

SparkQA commented Nov 17, 2015

Test build #46095 has finished for PR 9761 at commit b2e354a.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@marmbrus
Copy link
Contributor

marmbrus commented Nov 17, 2015

Thanks! Merging to master and 1.6.

asfgit pushed a commit that referenced this pull request Nov 17, 2015
@gatorsmile @marmbrus
[SPARK-8658][SQL][FOLLOW-UP] AttributeReference's equals method compa…
933f025 
…res all the members

Based on the comment of cloud-fan in #9216, update the AttributeReference's hashCode function by including the hashCode of the other attributes including name, nullable and qualifiers.

Here, I am not 100% sure if we should include name in the hashCode calculation, since the original hashCode calculation does not include it.

marmbrus cloud-fan Please review if the changes are good.

Author: gatorsmile <gatorsmile@gmail.com>

Closes #9761 from gatorsmile/hashCodeNamedExpression.

(cherry picked from commit 0158ff7)
Signed-off-by: Michael Armbrust <michael@databricks.com>
@asfgit asfgit closed this in 0158ff7 Nov 17, 2015
@gatorsmile gatorsmile deleted the hashCodeNamedExpression branch November 18, 2015 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@gatorsmile @SparkQA @nongli @marmbrus